-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Infrastructure: correct dependencies and clean helix agent from stray corerun procs #54094
Conversation
hoyosjs
commented
Jun 12, 2021
- Add dependency on coreclr in monojit/monointerpreter jobs explicitly to solve pipeline issue Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53842
- Use Helix Pre/Post hooks to kill lingering corerun instances to work around ARM64 agent issues
…to solve pipeline issue dotnet#53842
…around ARM64 agent issues
Tagging subscribers to this area: @hoyosjs Issue Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor suggestions.
@@ -68,6 +68,11 @@ | |||
<HelixPreCommand Condition="'$(TargetsWindows)' != 'true' and '$(BrowserHost)' != 'windows'" Include="export MONO_ENV_OPTIONS='$(MonoEnvOptions)'" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(TargetsWindows)' == 'true' or '$(BrowserHost)' == 'windows'"> | |||
<HelixPreCommand Include="taskkill.exe /f /im corerun.exe"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add similar command for unix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather get this in sooner rather than later given the stability issues. I am not sure if all unix systems we have can use pkill
, otherwise something like ps aux | grep -i corerun | grep -v grep | awk '{ print $2 }' | xargs kill -9
would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Lets get this in first.
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
cc: @dotnet/runtime-infrastructure |
Hello @hoyosjs! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Merging this as this is blocking all arm64 runs. |
@@ -64,6 +64,8 @@ jobs: | |||
- '${{ parameters.runtimeFlavor }}_common_test_build_p1_AnyOS_AnyCPU_${{parameters.buildConfig }}' | |||
- ${{ if ne(parameters.stagedBuild, true) }}: | |||
- ${{ if or( eq(parameters.runtimeVariant, 'minijit'), eq(parameters.runtimeVariant, 'monointerpreter')) }}: | |||
# This is needed for creating a CORE_ROOT in the current design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
courious, shouldn't this be when runtimeFlavor
is mono? that way we cover all mono flavors? Or are minjit
/monointerpreter
always a value for when we are running tests on mono?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the ones where I saw the issue, I thought about it, but I am not sure how the other mono runs work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naricc do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safern Its just that with minijit/monointerpreter, there is no seperate product build (we only have one build that has both of them). Other runtime variants do have their own package, thus it needs to be parameterized by the runtimeVariant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one question, LGTM